sds: additional support for symlink-based key rotation. by htuch · Pull Request #13721 · envoyproxy/envoy

htuch · 2020-10-23T00:07:50Z

There are a few limitations in our existing support for symlink-based
key rotation:

We don't atomically resolve symlinks, so a single snapshot might have
inconsistent symlink resolutions for different watched files.
Watches are on parent directories, e.g. for /foo/bar/baz on /foo/bar,
which doesn't support common key rotation schemes were /foo/new/baz
is rotated via a mv -Tf /foo/new /foo/bar.

The solution is to provide a structured WatchedDirectory for Secrets to
opt into when monitoring DataSources. SDS will used WatchedDirectory
to setup the inotify watch instead of the DataSource path. On update, it will
read key/cert twice, verifying file content hash consistency.

Risk level: Low (opt-in feature)
Testing: Unit and integration tests added.

Fixes #13663
Fixes #10979
Fixes #13370

Signed-off-by: Harvey Tuch htuch@google.com

repokitteh-read-only · 2020-10-23T00:07:56Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13721 was opened by htuch.

see: more, trace.

htuch · 2020-10-23T00:10:25Z

@mattklein123 @lizan @tsaarni can you take a look? This will address #13663, #10979 and #13370, I'd like to obtain consensus on the API before adding implementation and tests.

api/envoy/config/core/v3/base.proto

tsaarni

Regardless of working with only a subset of all DataSource use cases, I like this API.

This will work nicely with Kubernetes symlink swap as well, by leaving subdirectory optional 👍

api/envoy/config/core/v3/base.proto

api/envoy/extensions/transport_sockets/tls/v3/common.proto

mattklein123

At a high level LGTM with a few comments.

api/envoy/config/core/v3/base.proto

api/envoy/extensions/transport_sockets/tls/v3/common.proto

htuch · 2020-10-26T12:22:30Z

I've updated the API to avoid modifying DataSource, introducing a new concept of an opt-in WatchedDirectory. I've also added a draft implementation and a very basis integration test. I'd like to achieve agreement on API and design, at which point I can clean up the implementation and fill out the rest of the tests. @lizan @mattklein123 @tsaarni PTAL.

include/envoy/filesystem/filesystem.h

source/common/config/watched_directory.cc

api/envoy/config/core/v3/base.proto

htuch · 2020-10-27T02:24:16Z

This now uses hash-based comparison of files after reading twice and validated consistency. This is similar to what gRPC is doing.

tsaarni

Looks good to me after the unbound loop in SdsApi::onWatchUpdate() is resolved.

source/common/secret/sds_api.h

source/common/filesystem/posix/filesystem_impl.cc

api/envoy/config/core/v3/base.proto

mattklein123

Thanks agreed this seems like a better direction. Flushing out a few API questions/comments.

/wait

api/envoy/config/core/v3/base.proto

api/envoy/extensions/transport_sockets/tls/v3/common.proto

htuch · 2020-10-27T23:38:36Z

PTAL, if this makes sense now in terms of API and hash-based comparison implementation then I can fill in the missing tests/docs and move this out of draft.

mattklein123

From an API perspective this LGTM with one question. LMK when ready for review and I can take a look.

/wait

api/envoy/extensions/transport_sockets/tls/v3/common.proto

Signed-off-by: Harvey Tuch <htuch@google.com>

…. Fix format. Signed-off-by: Harvey Tuch <htuch@google.com>

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123

LGTM with some API comments, thank you!

/wait

api/envoy/extensions/transport_sockets/tls/v3/common.proto

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2020-11-13T14:23:49Z

@mattklein123 to deal with failure scenarios I had to add some stats, which then forced me to fix SDS stats more generally, since they were broken before (they all ended up anonymously in the root namespace). PTAL and LMK if there's anything else needed around stats here.

mattklein123

Thanks LGTM with small questions.

/wait-any

mattklein123 · 2020-11-13T19:27:20Z

api/envoy/extensions/transport_sockets/tls/v3/common.proto

+  // If specified, updates of a file-based *trusted_ca* source will be triggered
+  // by this watch. This allows explicit control over the path watched, by
+  // default the parent directory of the filesystem path in *trusted_ca* is
+  // watched if this field is not specified. This only applies when a
+  // *CertificateValidationContext* is delivered by SDS with references to
+  // filesystem paths.
+  config.core.v3.WatchedDirectory watched_directory = 11;


I still don't understand how this does anything if there is only a single file above? What's the utility?

Let's say you have a trusted CA files /foo/bar/baz/ca.pem. By default, envoy watches /foo/bar/baz. If you want to reload when baz is atomically moved, you need to set the watch on /foo/bar. This is what this field allows.

OK I think having a concrete example might help for this stuff (or ref link back to the docs where you have the example) as it was pretty hard for me to understand the difference.

source/common/secret/sds_api.cc

mattklein123

LGTM. Feel free to do any doc updates as a follow up.

htuch · 2020-11-13T21:23:02Z

Sure, will merge as some folks are depending on this, but I'll take an AI to do the docs followups next week. Thanks.

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <htuch@google.com>

Some followup docs tweaks to #13721. Signed-off-by: Harvey Tuch <htuch@google.com>

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <htuch@google.com>

…3721) There are a few limitations in our existing support for symlink-based key rotation: We don't atomically resolve symlinks, so a single snapshot might have inconsistent symlink resolutions for different watched files. Watches are on parent directories, e.g. for /foo/bar/baz on /foo/bar, which doesn't support common key rotation schemes were /foo/new/baz is rotated via a mv -Tf /foo/new /foo/bar. The solution is to provide a structured WatchedDirectory for Secrets to opt into when monitoring DataSources. SDS will used WatchedDirectory to setup the inotify watch instead of the DataSource path. On update, it will read key/cert twice, verifying file content hash consistency. Risk level: Low (opt-in feature) Testing: Unit and integration tests added. Fixes envoyproxy#13663 Fixes envoyproxy#10979 Fixes envoyproxy#13370 Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Qin Qin <qqin@google.com>

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Qin Qin <qqin@google.com>

repokitteh-read-only bot added the api label Oct 23, 2020

htuch assigned lizan and mattklein123 Oct 23, 2020

htuch commented Oct 23, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

tsaarni reviewed Oct 23, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

mattklein123 reviewed Oct 23, 2020

View reviewed changes

htuch force-pushed the sds-symlink branch from b843f06 to d2b2957 Compare October 26, 2020 12:20

htuch commented Oct 26, 2020

View reviewed changes

include/envoy/filesystem/filesystem.h Outdated Show resolved Hide resolved

htuch mentioned this pull request Oct 26, 2020

Symlink-atomic reads for private key/certificate pairs #13370

Closed

tsaarni reviewed Oct 26, 2020

View reviewed changes

source/common/config/watched_directory.cc Outdated Show resolved Hide resolved

markdroth reviewed Oct 26, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

tsaarni reviewed Oct 27, 2020

View reviewed changes

source/common/secret/sds_api.h Outdated Show resolved Hide resolved

source/common/filesystem/posix/filesystem_impl.cc Outdated Show resolved Hide resolved

markdroth reviewed Oct 27, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

mattklein123 requested changes Oct 27, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added waiting and removed waiting labels Oct 27, 2020

mattklein123 requested changes Oct 28, 2020

View reviewed changes

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Oct 28, 2020

htuch added 6 commits November 9, 2020 18:12

baseline from GH

08d0c51

Signed-off-by: Harvey Tuch <htuch@google.com>

Fine grained WatchedPath control, hash-based file atomic reads.

d3c88a5

Signed-off-by: Harvey Tuch <htuch@google.com>

Revert filesystem changes.

ce847fd

Signed-off-by: Harvey Tuch <htuch@google.com>

Always load secrets inline, even with non-watched path. Bound retries…

cbe018b

…. Fix format. Signed-off-by: Harvey Tuch <htuch@google.com>

Back to WatchedDirectory.

6ae5d80

Signed-off-by: Harvey Tuch <htuch@google.com>

Merge remote-tracking branch 'upstream/master' into sds-symlink

8a2056e

htuch changed the title ~~[WiP] sds/base: additional support for symlink-based key rotation.~~ sds: additional support for symlink-based key rotation. Nov 10, 2020

htuch added 4 commits November 11, 2020 20:36

Release notes and docs.

2282367

Signed-off-by: Harvey Tuch <htuch@google.com>

Typo.

f7b7d94

Signed-off-by: Harvey Tuch <htuch@google.com>

Fix clang-tidy.

da64842

Signed-off-by: Harvey Tuch <htuch@google.com>

fails_on_windows tag added (after consulting @wrowe)

2dae3be

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123 requested changes Nov 12, 2020

View reviewed changes

api/envoy/extensions/transport_sockets/tls/v3/common.proto Show resolved Hide resolved

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 12, 2020

htuch added 2 commits November 12, 2020 17:21

Strengthen docs.

6b5f720

Signed-off-by: Harvey Tuch <htuch@google.com>

Fix SDS stats and avoid crashing on empty directory rotations.

4ef3f13

Signed-off-by: Harvey Tuch <htuch@google.com>

repokitteh-read-only bot removed the waiting label Nov 13, 2020

mattklein123 reviewed Nov 13, 2020

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Nov 13, 2020

mattklein123 approved these changes Nov 13, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Nov 13, 2020

htuch merged commit 122257e into envoyproxy:master Nov 13, 2020

htuch added a commit to htuch/envoy that referenced this pull request Nov 15, 2020

sds: improve watched directory documentation.

b927378

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch mentioned this pull request Nov 15, 2020

sds: improve watched directory documentation. #14029

Merged

htuch added a commit that referenced this pull request Nov 16, 2020

sds: improve watched directory documentation. (#14029)

dac6e58

Some followup docs tweaks to #13721. Signed-off-by: Harvey Tuch <htuch@google.com>

andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020

sds: improve watched directory documentation. (envoyproxy#14029)

54af113

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <htuch@google.com>

qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020

sds: improve watched directory documentation. (envoyproxy#14029)

2fdd53c

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Qin Qin <qqin@google.com>

ggreenway mentioned this pull request May 13, 2021

Support reloading TlsCertificate files #10387

Open

Conversation

htuch commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Oct 23, 2020

Uh oh!

htuch commented Oct 23, 2020

Uh oh!

Uh oh!

tsaarni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented Oct 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented Oct 27, 2020

Uh oh!

tsaarni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch commented Oct 27, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

htuch Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Nov 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

htuch commented Oct 23, 2020 •

edited

Loading

tsaarni left a comment •

edited

Loading

htuch commented Nov 13, 2020 •

edited

Loading